Add `User-Agent` header to all outbound requests coming from the API #877

sarayourfriend · 2022-08-11T16:22:40Z

Fixes

Description

The fix should be applied to all outbound requests that could make a request to Wikimedia, not just in the Watermark endpoint. This is all of our outbound requests, so I've had to update all our outbound requests for it.

I tried to explore some ways of centralising this configuration, primarily by subclasses requests.Session and forcing a purpose kwarg to be present on all calls to ::request that would then configure the UA header, but it seemed heavy and I wasn't sure exactly how it would work or if folks would like that. I think it would make things a little easier to manage, or at least easier to not make mistakes with. But we also use grequests and urlopen which would still need their own bespoke solution... so I opted for the simplest approach across the board for now. We can improve this/generalise/abstract on it later if we want.

Testing Instructions

Run the app locally and ensure there are no issues.

Check out the unit tests. The ones involving urlopen were a complete nightmare to write!

Checklist

My pull request has a descriptive title (not a vague title like Update index.md).
My pull request targets the default branch of the repository (main) or a parent feature branch.
My commit messages follow best practices.
My code follows the established code style of the repository.
I added or updated tests for the changes I made (if applicable).
I added or updated documentation (if applicable).
I tried running the project locally and verified that there are no visible errors.

Developer Certificate of Origin

Developer Certificate of Origin
Version 1.1

Copyright (C) 2004, 2006 The Linux Foundation and its contributors.
1 Letterman Drive
Suite D4700
San Francisco, CA, 94129

Everyone is permitted to copy and distribute verbatim copies of this
license document, but changing it is not allowed.


Developer's Certificate of Origin 1.1

By making a contribution to this project, I certify that:

(a) The contribution was created in whole or in part by me and I
    have the right to submit it under the open source license
    indicated in the file; or

(b) The contribution is based upon previous work that, to the best
    of my knowledge, is covered under an appropriate open source
    license and I have the right under that license to submit that
    work with modifications, whether created in whole or in part
    by me, under the same open source license (unless I am
    permitted to submit under a different license), as indicated
    in the file; or

(c) The contribution was provided directly to me by some other
    person who certified (a), (b) or (c) and I have not modified
    it.

(d) I understand and agree that this project and the contribution
    are public and that a record of the contribution (including all
    personal information I submit with it, including my sign-off) is
    maintained indefinitely and may be redistributed consistent with
    this project or the open source license(s) involved.

github-actions · 2022-08-11T16:25:46Z

API Developer Docs Preview: Ready

https://wordpress.github.io/openverse-api/_preview/877

Please note that GitHub pages takes a little time to deploy newly pushed code, if the links above don't work or you see old versions, wait 5 minutes and try again.

You can check the GitHub pages deployment action list to see the current status of the deployments.

sarayourfriend · 2022-08-11T17:50:54Z

docker-compose.yml

@@ -18,7 +18,7 @@ services:
    environment:
      PORT: 8222
      MALLOC_ARENA_MAX: 2
-    command: ["-enable-url-source"]
+    command: ["-enable-url-source -forward-headers User-Agent"]


This new option will need to be ported up to the infrastructure repository as well, if it is merged.

dhruvkb

I think this is a reasonable configuration for now. The new env var you've added OUTBOUND_USER_AGENT_TEMPLATE should also be added to the env.template file (if only as a comment because the default value is good enough).

I agree that in the future we can use a central solution to manage requests but that will require all HTTP requests to first use the same libraries which will take a significant amount of time.

AetherUnbound

This is great, thank you for adding thorough tests too. Agreed that a more centralized implementation would be cool but the variety of request methods we have precludes that. I think the "purpose" interpolation you've added is excellent 💯

I have a few comments around the tests but the operational code is ✔️ IMO

api/test/unit/utils/watermark_test.py

AetherUnbound · 2022-08-12T21:52:19Z

api/test/unit/utils/validate_images_test.py

+
+    assert len(grequests.requests) > 0
+    for r in grequests.requests:
+        assert HEADERS == r.kwargs["headers"]


Nit: All the other tests use the format assert actual == expected whereas this one seems reversed.

AetherUnbound · 2022-08-12T21:58:29Z

api/test/unit/views/image_views_test.py

+
+
+@pytest.fixture(autouse=True)
+def requests(monkeypatch) -> RequestsFixture:


Just a thought, I notice that some of these fixtures differ slightly but a few of them (this one and the same fixture in watermark_test.py). Do you think it'd be possible to move some of these up to a common conftest.py so they only need to be defined once? Particularly with fixtures as simple as api_client.

I would rather do this refactor in another PR as part of another issue that more careful looks at the state of our unit test fixtures in general and thoughtfully establishes a pattern. For now most of these have subtle differences that would be tedious to abstract.

Sounds good 🙂

sarayourfriend · 2022-08-16T17:01:04Z

Blocked by #875

AetherUnbound

New changes look good 👍🏼

openverse-bot added 💻 aspect: code Concerns the software code in the repository 🛠 goal: fix Bug fix 🟧 priority: high Stalls work on the project or its dependents labels Aug 11, 2022

sarayourfriend commented Aug 11, 2022

View reviewed changes

dhruvkb approved these changes Aug 12, 2022

View reviewed changes

sarayourfriend force-pushed the fix/watermark-useragent branch from a1a3e53 to 2ac52a4 Compare August 12, 2022 16:41

sarayourfriend changed the base branch from main to add/better-status-code-thumbnail-rendering August 12, 2022 16:41

sarayourfriend marked this pull request as ready for review August 12, 2022 16:41

sarayourfriend requested a review from a team as a code owner August 12, 2022 16:41

sarayourfriend requested review from AetherUnbound and obulat and removed request for a team August 12, 2022 16:41

AetherUnbound approved these changes Aug 12, 2022

View reviewed changes

AetherUnbound mentioned this pull request Aug 15, 2022

Prevent tests from making network requests for link validation and fix potentially non-unique factory props #884

Merged

6 tasks

sarayourfriend force-pushed the add/better-status-code-thumbnail-rendering branch from 758f033 to 2695333 Compare August 16, 2022 16:51

sarayourfriend force-pushed the fix/watermark-useragent branch from 2ac52a4 to bad1118 Compare August 16, 2022 16:52

sarayourfriend added the ⛔ status: blocked Blocked & therefore, not ready for work label Aug 16, 2022

Base automatically changed from add/better-status-code-thumbnail-rendering to main August 16, 2022 17:08

sarayourfriend added 7 commits August 16, 2022 13:09

Do not 500 when upstream provider is unavailable

4c7b9fa

Remove invalid test case

ee4cc4f

Do not 500 when upstream provider is unavailable

0df7286

Use more descriptive not found error when watermark is disabled

9ecbf67

Add User-Agent header to all outbound requests

ee8d30f

Enable forwarding UA header in imaginary

b057ffb

Add tests for validate images requests

7d28f47

sarayourfriend added 6 commits August 16, 2022 13:09

Add tests for watermark requests

3b5c656

Add tests for waveform requests

8b6c644

Add tests for oembed requests

8a301ff

Add tests for thumbnail requests

db24662

Add tests for thumbnail requests

29f562d

Add tests for thumbnail requests

e5b4c8b

sarayourfriend force-pushed the fix/watermark-useragent branch from bad1118 to e5b4c8b Compare August 16, 2022 17:09

sarayourfriend removed the ⛔ status: blocked Blocked & therefore, not ready for work label Aug 16, 2022

sarayourfriend added 2 commits August 16, 2022 13:48

Fix bad merge

06a45fd

Move thumbnail tests out of integration and into unit

85275fd

sarayourfriend requested review from AetherUnbound and dhruvkb August 17, 2022 16:44

AetherUnbound approved these changes Aug 17, 2022

View reviewed changes

sarayourfriend merged commit f0c44d5 into main Aug 17, 2022

sarayourfriend deleted the fix/watermark-useragent branch August 17, 2022 17:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `User-Agent` header to all outbound requests coming from the API #877

Add `User-Agent` header to all outbound requests coming from the API #877

sarayourfriend commented Aug 11, 2022 •

edited

Loading

github-actions bot commented Aug 11, 2022 •

edited

Loading

sarayourfriend Aug 11, 2022

dhruvkb left a comment

AetherUnbound left a comment

AetherUnbound Aug 12, 2022

AetherUnbound Aug 12, 2022

sarayourfriend Aug 16, 2022

AetherUnbound Aug 16, 2022

sarayourfriend commented Aug 16, 2022

AetherUnbound left a comment



		@pytest.fixture(autouse=True)
		def requests(monkeypatch) -> RequestsFixture:

Add User-Agent header to all outbound requests coming from the API #877

Add User-Agent header to all outbound requests coming from the API #877

Conversation

sarayourfriend commented Aug 11, 2022 • edited Loading

Fixes

Description

Testing Instructions

Checklist

Developer Certificate of Origin

github-actions bot commented Aug 11, 2022 • edited Loading

sarayourfriend Aug 11, 2022

Choose a reason for hiding this comment

dhruvkb left a comment

Choose a reason for hiding this comment

AetherUnbound left a comment

Choose a reason for hiding this comment

AetherUnbound Aug 12, 2022

Choose a reason for hiding this comment

AetherUnbound Aug 12, 2022

Choose a reason for hiding this comment

sarayourfriend Aug 16, 2022

Choose a reason for hiding this comment

AetherUnbound Aug 16, 2022

Choose a reason for hiding this comment

sarayourfriend commented Aug 16, 2022

AetherUnbound left a comment

Choose a reason for hiding this comment

Add `User-Agent` header to all outbound requests coming from the API #877

Add `User-Agent` header to all outbound requests coming from the API #877

sarayourfriend commented Aug 11, 2022 •

edited

Loading

github-actions bot commented Aug 11, 2022 •

edited

Loading